-
-
Notifications
You must be signed in to change notification settings - Fork 18k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH: consistency of input args for boundaries in DataFrame.between_time() #40245 #43248
ENH: consistency of input args for boundaries in DataFrame.between_time() #40245 #43248
Conversation
suyashgupta01
commented
Aug 27, 2021
•
edited
Loading
edited
- xref ENH: consistency of input args for boundaries #40245
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove .github/.pre-commit-config.yaml
79bcde4
to
6e9882d
Compare
@MarcoGorelli can you approve the workflows, please? |
@jreback It's all green, take a look :) |
@jreback Are any other changes required? |
lgtm @attack68 @MarcoGorelli if any comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of nitpicks left, but the rest looks good to me!
pandas/core/generic.py
Outdated
inclusive = "both" | ||
elif inclusive not in ["both", "neither", "left", "right"]: | ||
raise ValueError( | ||
f"Inclusive has to be either string of 'both'," |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f"Inclusive has to be either string of 'both'," | |
f"Inclusive has to be either string of 'both', " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the test will need updating here as well
Co-authored-by: Marco Edward Gorelli <marcogorelli@protonmail.com>
Co-authored-by: Marco Edward Gorelli <marcogorelli@protonmail.com>
@MarcoGorelli Made the requested changes, please take a look :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me pending green, thanks @suyashgupta01 !
@attack68 your review's pending, please take a look :) |
@MarcoGorelli Can you please take a look at the failing check, I can't figure out why it is failing? |
it's unrelated, don't worry about it |
thanks @suyashgupta01 ver nice |
…exer_between_time` for pandas 2.0.0 & enabling more tests ### What changes were proposed in this pull request? This PR proposes to support `DatetimeIndex.indexer_between_time` to support pandas 2.0.0 and above. See pandas-dev/pandas#43248 for more detail. This PR also enables bunch of tests for `Series`, `Index` and `GroupBy`. ### Why are the changes needed? To match the behavior with latest pandas. ### Does this PR introduce _any_ user-facing change? `DatetimeIndex.indexer_between_time` now has the same behavior with the latest pandas. ### How was this patch tested? Enabling & updating the existing UTs and doctests. Closes #42533 from itholic/enable-many-tests. Authored-by: itholic <haejoon.lee@databricks.com> Signed-off-by: Ruifeng Zheng <ruifengz@apache.org>
…exer_between_time` for pandas 2.0.0 & enabling more tests ### What changes were proposed in this pull request? This PR proposes to support `DatetimeIndex.indexer_between_time` to support pandas 2.0.0 and above. See pandas-dev/pandas#43248 for more detail. This PR also enables bunch of tests for `Series`, `Index` and `GroupBy`. ### Why are the changes needed? To match the behavior with latest pandas. ### Does this PR introduce _any_ user-facing change? `DatetimeIndex.indexer_between_time` now has the same behavior with the latest pandas. ### How was this patch tested? Enabling & updating the existing UTs and doctests. Closes apache#42533 from itholic/enable-many-tests. Authored-by: itholic <haejoon.lee@databricks.com> Signed-off-by: Ruifeng Zheng <ruifengz@apache.org>
…exer_between_time` for pandas 2.0.0 & enabling more tests ### What changes were proposed in this pull request? This PR proposes to support `DatetimeIndex.indexer_between_time` to support pandas 2.0.0 and above. See pandas-dev/pandas#43248 for more detail. This PR also enables bunch of tests for `Series`, `Index` and `GroupBy`. ### Why are the changes needed? To match the behavior with latest pandas. ### Does this PR introduce _any_ user-facing change? `DatetimeIndex.indexer_between_time` now has the same behavior with the latest pandas. ### How was this patch tested? Enabling & updating the existing UTs and doctests. Closes apache#42533 from itholic/enable-many-tests. Authored-by: itholic <haejoon.lee@databricks.com> Signed-off-by: Ruifeng Zheng <ruifengz@apache.org>